Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate Keycloak Admin Client from config classes to the @ConfigMapping #39342

Conversation

michalvavrik
Copy link
Member

part of the #39185

Both extensions are not used by any Quarkiverse extension, I don't believe users have many reasons to actually inject the config directly and if so, you can see that migration is straightforward from the changes in this PR.

https://mvnrepository.com/artifact/io.quarkus/quarkus-keycloak-admin-client/usages
https://mvnrepository.com/artifact/io.quarkus/quarkus-keycloak-admin-client-reactive/usages

Copy link

quarkus-bot bot commented Mar 11, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 36b0bbd.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@michalvavrik michalvavrik requested a review from sberyozkin March 11, 2024 23:16
@geoand geoand requested a review from radcortez March 12, 2024 06:49
@sberyozkin sberyozkin merged commit 926db7c into quarkusio:main Mar 12, 2024
19 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Mar 12, 2024
@michalvavrik michalvavrik deleted the feature/migrate-keycloak-admin-client-config-classes branch March 12, 2024 10:42
@famod
Copy link
Member

famod commented Mar 15, 2024

This might be worth a note in the 3.9 migration guide:
While trying out 3.9.0.CR2 I got compile errors w.r.t. KeycloakAdminClientConfig.realm (and others). I had to switch to .realm().

@michalvavrik
Copy link
Member Author

This might be worth a note in the 3.9 migration guide: While trying out 3.9.0.CR2 I got compile errors w.r.t.

+1 but someone with write rights must do that

KeycloakAdminClientConfig.realm (and others). I had to switch to .realm().

quick check - it's inside application, not an extension, right? I didn't think user applications has many reason to inject this. can you please give me a hint why it was useful? thank you

@famod
Copy link
Member

famod commented Mar 15, 2024

KeycloakAdminClientConfig.realm (and others). I had to switch to .realm().

quick check - it's inside application, not an extension, right? I didn't think user applications has many reason to inject this. can you please give me a hint why it was useful? thank you

E.g.:

/**
 * Access to keycloak user rest api client. Marked with {@link RestClientWrapper} to support interception.
 */
@ApplicationScoped
@RestClientWrapper
@RequiredArgsConstructor(access = AccessLevel.PROTECTED)
public class KeycloakUserApiClient {

    private final KeycloakAdminClientConfig adminclientConfig;
    private final Keycloak adminClient;

    // ...

    /**
     * Create new user.
     *
     * @param userRepresentation user data
     */
    public void create(final UserRepresentation userRepresentation) {
        api().users().create(userRepresentation);
    }

    private RealmResource api() {
        return adminClient.realm(adminclientConfig.realm());
    }
}

PS: RestClientWrapper is a custom annotation of ours.

@michalvavrik
Copy link
Member Author

michalvavrik commented Mar 15, 2024

Sure, I think that makes sense if you write API that is reused by applications that can set different realms. Thank you

My opinion is that we should keep the change as it's for a good and migration is super easy (parentheses), but add it to migration guide.

@michalvavrik
Copy link
Member Author

@sberyozkin please add a note to migration guide

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants